-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611
Conversation
@sgreene570: This pull request references Bugzilla bug 1954330, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@sgreene570: This pull request references Bugzilla bug 1954330, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Are you sure? That is surprising to me. Can you add a unit test for this? |
Yup. I was also surprised by this behavior. |
Commit `ea085e7` added logic to reconcile the Ingress namespace during cluster upgrades. This follow-up commit fixes some mistakes introduced by that commit. pkg/operator/controller/ingress/namespace.go: Fix a typo: `opensift.io/cluster-monitoring`. We are, after all, in the computing business, and not the baking business. Check if annotation/label map values exist since comparing a non-existent map value to the empty string will always return true in this case. pkg/operator/controller/ingress/namespace_test.go: Add unit test to prove that a map value that does not exist is equivalent to the empty string. Rename the `original` namespace to `desired`. Switch ordering of `desired` and `mutated` in `routerNamespaceChanged` calling sites to better reflect the intent of the unit tests.
@sgreene570: This pull request references Bugzilla bug 1954330, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@sgreene570: An error was encountered querying GitHub for users with public email (asood@redhat.com) for bug 1954330 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#abuse-rate-limits\",\n \"message\": \"You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
tc.mutate(mutated) | ||
if changed, updated := routerNamespaceChanged(original, mutated); changed != tc.expect { | ||
if changed, updated := routerNamespaceChanged(mutated, desired); changed != tc.expect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my thinking, the names were fine before: routerNamespaceChanged
has parameters current
and expected
, and its logic copies current
and updates it using expected
. In actual use, current
is the object that's in the API, and expected
is the new desired object, which mutates the current API object based on some update (changes to the ingresscontroller, or changes to the operator itself as in the case of these new annotations and labels). I don't think the order really matters here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In actual use,
current
is the object that's in the API, andexpected
is the new desired object
Right, the only caveat being in the case of the ingress namespace resource, desired is effectively manifests.RouterNamespace
. I think that's where the prior naming was throwing me off.
If you don't mind, I would prefer to use the new naming and ordering in this PR, since IMO, it makes the test added in this PR simpler.
Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sgreene570: All pull requests linked via external trackers have merged: Bugzilla bug 1954330 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.7 |
@sgreene570: #611 failed to apply on top of branch "release-4.7":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Commit
ea085e7
added logic to reconcile the Ingress namespace duringcluster upgrades. This follow-up commit fixes some mistakes introduced
by that commit.
pkg/operator/controller/ingress/namespace.go:
Fix a typo:
opensift.io/cluster-monitoring
.We are, after all, in the computing business, and not the baking
business.
Check if annotation/label map values exist since comparing a
non-existent map value to the empty string will always return true
in this case.
pkg/operator/controller/ingress/namespace_test.go:
Add unit test to prove that a map value that does not exist
is equivalent to the empty string.
Rename the
original
namespace todesired
.Switch ordering of
desired
andmutated
inrouterNamespaceChanged
calling sites to better reflect the intent of the unit tests.
This is a follow up to #608, which merged with some mistakes that were missed during review.